Add ambient-loader proposal #63

arcanis · 2022-02-10T11:49:24Z

This design document is a follow-up to #58

Fixes #56

doc/design/proposal-ambient-loaders.md

GeoffreyBooth · 2022-02-13T00:25:26Z

doc/design/proposal-ambient-loaders.md

+
+If loaders cannot generally influence each other, we could have a subset of them do. Indeed, not all loaders affect the resolution so much that they are a requirement to followup loaders. We could have two levels of loaders:
+
+- Ambient loaders would be defined via the `--ambient-loader <module>` flag. They would be loaded sequentially, and would affect the resolution of all followup loaders.


If I registered two ambient loaders, would the first one affect the resolution of the second?

Oh, wait, what. I did not get that from this doc. I understood ambient loaders to work in tandem like how regular loaders do (one regular loader does not affect the other), just specifically when loading a regular loader.

Suggested change

- Ambient loaders would be defined via the `--ambient-loader <module>` flag. They would be loaded sequentially, and would affect the resolution of all followup loaders.

- Ambient loaders would be defined via the `--ambient-loader <module>` flag. They would be loaded sequentially and would affect the resolution of all subsequent ambient loaders.

or possibly

Suggested change

- Ambient loaders would be defined via the `--ambient-loader <module>` flag. They would be loaded sequentially, and would affect the resolution of all followup loaders.

- Ambient loaders would be defined via the `--ambient-loader <module>` flag. They would be loaded sequentially and would affect the resolution of all subsequent ambient and regular loaders.

Making ambient loaders affect each other sounds like a pandora's box.

Updated with your second wording

Making ambient loaders affect each other sounds like a pandora's box.

Why that?

Updated with your second wording

Thanks!

Making ambient loaders affect each other sounds like a pandora's box.
Why that?

Wouldn't that open the door for reentry?

To be honest I'm still not sure I see a problem with reentrance, we may have different behaviours in mind 😃

But in this case it shouldn't be reentrant anyway: it's just using the result of the previous loader to load the next one, so all execution occurs in the same order and we don't go twice in the same loader during the same resolution pass.

What about when one ambient loader depends on another in a circular fashion?

Do you have an example?

The closest I can think of would be if, say, we had /foo.zip/bar.tgz/qux.zip/index.js, but isn't this pattern true as well for regular module imports?

doc/design/proposal-ambient-loaders.md

JakobJingleheimer

Proposal doc LGTM, thanks!

~~EDIT: Please update current line 19 (defining ambient loaders affects on each other) to specifically call out that they affect the resolution of other ambient loaders.~~ (done in 706ea20)

Co-authored-by: Geoffrey Booth <456802+GeoffreyBooth@users.noreply.github.com>

targos · 2022-02-15T18:36:25Z

doc/design/proposal-ambient-loaders.md

+
+Indeed, importing `ts-node` would require the `zip` loader to contribute to the resolution (so that the final path loaded is `$PROJECT/node_modules.zip/ts-node` instead of `$PROJECT/node_modules/ts-node`, which doesn't exist), but it’s not what happens today: both `zip` and `ts-node` are resolved by the default Node resolver, preventing `ts-node` from resolving.
+
+The reason for that is an attempt to keep loaders as isolated as possible, to allow followup improvements like reusing them across workers or scaling them up. If all loaders were influenced by prior loaders, they’d effectively coalesce into a single one, which would prevent such work. Still, the problem remains.


Are we 100% certain that we cannot let regular loaders be influenced by prior loaders? I'm afraid that if ambient loaders seem more powerful than regular loaders, we will just end up with everyone always using --ambient-loader and forget about the other flag.

Are we 100% certain that we cannot let regular loaders be influenced by prior loaders?

I think we said no because it would require reentrancy, and our preliminary thoughts on this Ambient Loader concept was that it's the nuclear option so it would be chosen very infrequently (too easy to foot-gun, so only worth it when you really need it and really know what you're doing).

I'd be curious to hear about reentrancy hazards that will crop up in the types of loaders that can be non-ambient. E.g. a use-case where you don't need --ambient-loader and yet are doing something tricky enough that you're likely to footgun. A simple source->source compiler such as a coffeescript loader wouldn't have any of these risks, I don't think.

@arcanis If you feel comfortable explaining it, do you mind adding a section to the document about reentrancy and its hazards and what this means for loaders/ambient loaders? Like perhaps how those issues can be addressed (or not) via ambient loaders, etc. Or what if we only allowed just one ambient loader?

As I mentioned in this thread, I'm not sure myself I see the problems with reentrancy (nor I see it avoidable for resolution in general, or particularly affected by ambient loaders), so I'd prefer if someone else could make a writeup.

Who is concerned about these hypothetical reentrancy concerns? From an outside perspective, it appears as if perhaps it was hypothesized but no one could think of any examples.

Who is concerned about these hypothetical reentrancy concerns? From an outside perspective, it appears as if perhaps it was hypothesized but no one could think of any examples.

@bmeck was the one who knew the most about them.

I'm also concerned about reentrancy, but for me that doesn't block adding the proposal itself, just a concern about a later stage.

btw, I'm not blocking this proposal in any way. I do not have the bandwidth to participate in meetings, so I try to at least follow what's happening in this repo. I trust that you all are doing the right thing in the end 👍🏻

guybedford · 2022-03-02T17:36:01Z

@arcanis could the use cases be supported by permitting defining this property in the loader itself?

export const ambient = true;

cspotcode · 2022-03-04T04:09:43Z

You'll be requiring node to load loaders sequentially, which I think is fine and is probably something we should do anyway, for all loaders.

node --loader a --loader b

If a is ambient, then b cannot be loaded until a is finished. And node will not know if a is ambient until importing it and checking the ambient export.

cspotcode · 2022-03-23T18:33:04Z

doc/design/proposal-ambient-loaders.md

+
+```
+node --ambient-loader zip
+     --ambient-loader pnp


I believe there is some debate about the correct ordering of these, based on Slack discussion:

https://node-js.slack.com/archives/CEVD3CX33/p1647532211808939

The competing factors are a) which ordering is more intuitive, and b) if ordering should be done such that argv takes precedence over NODE_OPTIONS

Actually, thinking about this some more, maybe the ordering in this particular example doesn't matter.

doc/design/proposal-ambient-loaders.md

Co-authored-by: Geoffrey Booth <456802+GeoffreyBooth@users.noreply.github.com>

GeoffreyBooth · 2022-03-25T16:07:51Z

I think this is good to merge in, unless there are any objections? Merging doesn’t mean that it gets on the roadmap, just that it’s a proposal under consideration; and future PRs can further improve it.

JakobJingleheimer · 2022-03-26T16:37:44Z

You'll be requiring node to load loaders sequentially, which I think is fine and is probably something we should do anyway, for all loaders.

node --loader a --loader b

If a is ambient, then b cannot be loaded until a is finished. And node will not know if a is ambient until importing it and checking the ambient export.

@cspotcode a bit of an implementation detail, but I think we could handle this in ESMLoader::pluck() (wherein a loader's exports are inspected and hooks etc are queued): we'd create a separate collection for ambient loaders.

cspotcode · 2022-03-26T16:47:51Z

Oh interesting idea. If someone did export const ambient = false; then it would still trigger that logic I think? It's a weird case, but something that would have to be documented.

The flow would be:

Resolve and inspect a's exports via pluck()
If ambient export is present, then we know we cannot resolve b until a has evaluated, since a might affect the resolution of b

JakobJingleheimer · 2022-03-26T16:50:00Z

Correct 🙂 (to clarify: that export const ambient = false would NOT cause it to be added to the ambient collection, only when true)

JakobJingleheimer · 2022-03-26T22:04:53Z

Gah, actually that wouldn't be so straightforward: a loader that needs an ambient loader would be unreadable, so the initial parse of the lay loader would fail before it gets to ESMLoader::pluck(). If we wanted to go the way of an export, we'd need to temporarily ignore failures and re-try until all loaders have been successfully parsed. I think this would get too ugly to be viable.

I think that doesn't block merging this proposal though (which doesn't depend on that).

cspotcode · 2022-03-26T22:42:51Z

Agreed, that's what I was pointing out: b can't resolve until a is fully loaded. So it would only be a performance optimization with gotchas: as soon as you've proven that a is not ambient, you can proceed with resolving the next loader.

cspotcode · 2022-03-26T22:50:41Z

And actually, export const ambient might rely on ambient loaders being listed strictly before (after?) non-ambient loaders in the command-line / NODE_OPTIONS args. Would get messy to do --loader ambient1 --loader b --loader ambient2; is ambient2 meant to affect b?

arcanis added 2 commits February 10, 2022 12:48

Adds the ambient-loader proposal

ed0fab9

Update proposal-ambient-loaders.md

96ecc5e

GeoffreyBooth added the loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team label Feb 10, 2022

mhdawson mentioned this pull request Feb 11, 2022

Node.js Loaders Team Meeting 2022-02-15 #64

Closed

targos reviewed Feb 12, 2022

View reviewed changes

doc/design/proposal-ambient-loaders.md Outdated Show resolved Hide resolved

GeoffreyBooth reviewed Feb 13, 2022

View reviewed changes

JakobJingleheimer approved these changes Feb 15, 2022

View reviewed changes

arcanis and others added 2 commits February 15, 2022 18:23

Update doc/design/proposal-ambient-loaders.md

706ea20

Co-authored-by: Geoffrey Booth <456802+GeoffreyBooth@users.noreply.github.com>

Update proposal-ambient-loaders.md

f403fd6

targos reviewed Feb 15, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

arcanis changed the title ~~Adds the ambient-loader proposal~~ Add ambient-loader proposal Feb 16, 2022

mhdawson mentioned this pull request Feb 25, 2022

Node.js Loaders Team Meeting 2022-03-01 #69

Closed

mhdawson mentioned this pull request Mar 11, 2022

Node.js Loaders Team Meeting 2022-03-15 #71

Closed

GeoffreyBooth removed the loaders-agenda Issues and PRs to discuss during the meetings of the Loaders team label Mar 13, 2022

Adds requested tidbits

630173d

cspotcode reviewed Mar 23, 2022

View reviewed changes

GeoffreyBooth reviewed Mar 24, 2022

View reviewed changes

doc/design/proposal-ambient-loaders.md Outdated Show resolved Hide resolved

Update doc/design/proposal-ambient-loaders.md

f4c816b

Co-authored-by: Geoffrey Booth <456802+GeoffreyBooth@users.noreply.github.com>

GeoffreyBooth approved these changes Mar 25, 2022

View reviewed changes

GeoffreyBooth merged commit e648658 into main Mar 27, 2022

GeoffreyBooth deleted the mael/ambient-loaders branch March 27, 2022 02:00

JakobJingleheimer mentioned this pull request May 9, 2022

Node.js Loaders Team Meeting 2022-05-10 #78

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ambient-loader proposal #63

Add ambient-loader proposal #63

arcanis commented Feb 10, 2022 •

edited

Loading

GeoffreyBooth Feb 13, 2022

arcanis Feb 13, 2022

JakobJingleheimer Feb 15, 2022 •

edited

Loading

arcanis Feb 15, 2022

JakobJingleheimer Feb 15, 2022

arcanis Feb 15, 2022

JakobJingleheimer Feb 15, 2022

arcanis Feb 15, 2022

JakobJingleheimer left a comment •

edited

Loading

targos Feb 15, 2022

JakobJingleheimer Feb 15, 2022 •

edited

Loading

cspotcode Feb 15, 2022

GeoffreyBooth Feb 16, 2022

arcanis Feb 16, 2022

cspotcode Feb 28, 2022

GeoffreyBooth Feb 28, 2022

JakobJingleheimer Mar 1, 2022

targos Mar 2, 2022

This comment was marked as resolved.

guybedford commented Mar 2, 2022

cspotcode commented Mar 4, 2022

cspotcode Mar 23, 2022

cspotcode Mar 23, 2022

GeoffreyBooth commented Mar 25, 2022

JakobJingleheimer commented Mar 26, 2022

cspotcode commented Mar 26, 2022

JakobJingleheimer commented Mar 26, 2022

JakobJingleheimer commented Mar 26, 2022 •

edited

Loading

cspotcode commented Mar 26, 2022

cspotcode commented Mar 26, 2022


		If loaders cannot generally influence each other, we could have a subset of them do. Indeed, not all loaders affect the resolution so much that they are a requirement to followup loaders. We could have two levels of loaders:

		- Ambient loaders would be defined via the `--ambient-loader <module>` flag. They would be loaded sequentially, and would affect the resolution of all followup loaders.


		Indeed, importing `ts-node` would require the `zip` loader to contribute to the resolution (so that the final path loaded is `$PROJECT/node_modules.zip/ts-node` instead of `$PROJECT/node_modules/ts-node`, which doesn't exist), but it’s not what happens today: both `zip` and `ts-node` are resolved by the default Node resolver, preventing `ts-node` from resolving.

		The reason for that is an attempt to keep loaders as isolated as possible, to allow followup improvements like reusing them across workers or scaling them up. If all loaders were influenced by prior loaders, they’d effectively coalesce into a single one, which would prevent such work. Still, the problem remains.

Add ambient-loader proposal #63

Add ambient-loader proposal #63

Conversation

arcanis commented Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobJingleheimer Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobJingleheimer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobJingleheimer Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

guybedford commented Mar 2, 2022

cspotcode commented Mar 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeoffreyBooth commented Mar 25, 2022

JakobJingleheimer commented Mar 26, 2022

cspotcode commented Mar 26, 2022

JakobJingleheimer commented Mar 26, 2022

JakobJingleheimer commented Mar 26, 2022 • edited Loading

cspotcode commented Mar 26, 2022

cspotcode commented Mar 26, 2022

arcanis commented Feb 10, 2022 •

edited

Loading

JakobJingleheimer Feb 15, 2022 •

edited

Loading

JakobJingleheimer left a comment •

edited

Loading

JakobJingleheimer Feb 15, 2022 •

edited

Loading

JakobJingleheimer commented Mar 26, 2022 •

edited

Loading